Allow serial_diff under min_doc_count aggs #86401
Conversation
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Hi @nik9000, I've created a changelog YAML for you. |
Before 6.6 we allowed the `serial_diff` agg in lots of places, including under `date_histogram` aggregations with `min_doc_count: 1`. This allowed you to take the difference of two adjacent buckets, skipping any that don't have any value. So if you have a value at 10am, no value at 11am, and another at noon the `serial_diff` will diff the 10am and noon values. In 6.6 we disabled support for this. We'd like it back.
|
Pinging @elastic/clients-team (Team:Clients) |
|
I took the opportunity to remove an annoying TODO with a bunch of |
| @@ -216,6 +209,11 @@ public void addBucketPathValidationError(String error) { | |||
| */ | |||
| public abstract void validateParentAggSequentiallyOrdered(String type, String name); | |||
|
|
|||
| /** | |||
| * Validates that the parent is sequentially ordered. | |||
There was a problem hiding this comment.
There's a method right above that has the same javadoc. I know these two methods don't do the same thing, so they should explain how they're different.
There was a problem hiding this comment.
👍 I meant to do this but skipped it. thanks.
| @@ -134,6 +131,11 @@ public void validateParentAggSequentiallyOrdered(String type, String name) { | |||
| + "] must have a histogram, date_histogram or auto_date_histogram as parent but doesn't have a parent" | |||
| ); | |||
| } | |||
|
|
|||
| @Override | |||
| public void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name) { | |||
There was a problem hiding this comment.
This needs javadoc. It's subtly different from the version in ForInsideTree, and it's not clear why they do two different things without loading the whole context.
There was a problem hiding this comment.
It always throws an exception. I'll jiggle it so it's more clear.
| @Override | ||
| protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) { | ||
| if (minDocCount != 0) { | ||
| addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0"); |
There was a problem hiding this comment.
Do we need to think about extended/hard bounds here? I'm pretty sure the answer is "no", but I want to say it out loud.
There was a problem hiding this comment.
We never have in the past. I think it's ok because those'll just make the result wider without adding any gaps.
nik9000
added
auto-merge-without-approval
|
run elasticsearch-ci/part-2 |
Before 6.6 we allowed the `serial_diff` agg in lots of places, including under `date_histogram` aggregations with `min_doc_count: 1`. This allowed you to take the difference of two adjacent buckets, skipping any that don't have any value. So if you have a value at 10am, no value at 11am, and another at noon the `serial_diff` will diff the 10am and noon values. In 6.6 we disabled support for this. We'd like it back.
Before 6.6 we allowed the `serial_diff` agg in lots of places, including under `date_histogram` aggregations with `min_doc_count: 1`. This allowed you to take the difference of two adjacent buckets, skipping any that don't have any value. So if you have a value at 10am, no value at 11am, and another at noon the `serial_diff` will diff the 10am and noon values. In 6.6 we disabled support for this. We'd like it back.
My change elastic#86401 never made it into 7.17.4 so the test was confused. This bumps the version. It'll make it into 7.17.5.
Before 6.6 we allowed the
serial_diffagg in lots of places, includingunder
date_histogramaggregations withmin_doc_count: 1. Thisallowed you to take the difference of two adjacent buckets, skipping any
that don't have any value. So if you have a value at 10am, no value at
11am, and another at noon the
serial_diffwill diff the 10am and noonvalues. In 6.6 we disabled support for this. We'd like it back.